-
Notifications
You must be signed in to change notification settings - Fork 33
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PPI-203 : deprecate context manager and implement attach/detach APIs #196
Conversation
4460fa5
to
1a39e25
Compare
@Deprecated( | ||
'This method will be removed in 0.19.0. Use [contextWithSpan] instead.') | ||
Context withSpan(Span span); | ||
Context withSpan(Span span) => contextWithSpan(this, span); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it's worth re-evaluating some of these deprecated instance methods now that we've agreed to use the static methods on Context
for things like attach/detach and treat Context
as its own immutable propagation context.
When I was reading through the examples above, I found things like this a bit confusing/daunting:
final rootToken = Context.attach(
contextWithSpan(Context.current, tracer.startSpan('root-1')..end()));
As a consumer, I might wonder when I should use a static method like Context.attach
versus a top-level helper function like contextWithSpan
. That example also uses contextWithSpan(Context.current, ...)
twice, which makes me wonder – if that's a common pattern, is there a way we can streamline it and avoid the user needing to take a roundabout way to create a new context from the current context with a given span?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like OTEL really needs a major to clean out the deprecations. It's gotten pretty bad and difficult to think through given the quantity of deprecated code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Context
is its own immutable propagation context as it is implemented now. I would like to refrain from muddying its APIs with unnecessary methods about spans. Spans need context, context doesn't need spans. So stuff like contextWithSpan() or spanFromContext() are cross-cutting APIs vs the static methods that pertain to context alone.
I agree with you that having the cross-cutting APIs as top-level functions while the context specific APIs are static methods on the Context
class is confusing, especially without keeping this distinction in mind. But if we want to colocate the functions, I'd opt for moving the context specific static APIs out of the class before moving cross-cutting APIs into the class.
I think there are common patterns that can be a little clunky, like you've demonstrated. In my option:
- It's demonstrating what is happening, and how it works. No need to guess, your intuition will suffice
- Additional APIs would require maintaining additional APIs.
- If consumers want something stream lined, I think it's reasonable to expect the consumer to define helper functions.
- I might change my opinion once Dart 2 support is thrown out. Defining functions that return a record as a tuple of context and span, mimicking golang APIs, would be clean. But I would not want to add a new type or update an interface to support this behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 that's helpful, thanks Blake!
abstract class ContextManager { | ||
/// Different implementations of [ContextManager] can be registered to use | ||
/// different underlying storage mechanisms. | ||
@Deprecated('This class will be removed in 0.19.0 without replacement.') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a blocker-comment, just clarifying my understanding and if correct, want us to be aware:
Are we deviating from the OTEL spec by deprecating the context manager? It seems like the attach and detach context approach is analogous to setting the active context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those links are not to the spec, but to the JavaScript implementation of the spec. The context spec is this document: https://opentelemetry.io/docs/specs/otel/context/. Getting a global, attaching, and detaching context are mentioned in the optional global operations section
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To touch on context manager specifically:
- It isn't required spec, only how the JavaScript implementation decided to organize themselves.
- Other languages do not have a context manager.
- I believe the main reason for the context manager abstraction is that Zones are not standard library for JavaScript. So the context manager is a type of inversion of control. The consumer can decide if they want to build their bundle with zones or not. And ofc this is not relevant to Dart since Zones are a part of the standard library.
QA +1 examples demonstrate working behavior: attach detach example output:
stream example output:
|
@Workiva/release-management-p |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 from RM
Which problem is this PR solving?
Fixes #185
The current context API is unnecessarily complex and does not support the attach/detach optional APIs.
In async programming, the attach/detach APIs can be incredibly handy in passing context to async processes that cannot propagate context manually (most likely due to public or otherwise unbreakable APIs). Zones can help in this case, but do not always suffice since zones might not appropriately follow code execution. Specifically, if a process includes putting an event on a stream/sink where that event is consumed by a stream listener, the zone does not propagate from where the event was placed on the stream/sink to the stream listener's callback.
Short description of the change
The current context implementation requires a modifiable concrete implementation used to decide how to propagate context. This is an adaption to OpenTelemetry JavaScript's implementation.
However, unlike JavaScript, zones are a language native feature: its type and implementation provided by the standard library. Therefore it is okay to assume that zones are available and thus simplify the context propagation story as compared to OpenTelemetry JavaScript.
A potential solution to this end would be to replace the abstract
Context
with the concreteZoneContext
. This solution was avoided becauseZoneContext
presumes that zone values are what carries the context. This means that manual propagation would be passing around the zone itself (or rather the contrived wrapper of the zone).Instead, the changes make
Context
its own immutable propagation context. This allows a zone, also an immutable propagation context, to hold both a stack of versions (due to multiple calls to attach) or a specific version (when binding a version to a zone for zone execution).tl;dr;
Context
is a stack implemented as a linked list. Cross-cutting APIs allow stacking the stack (attach/detach) on a zone, or binding a specific version (linked list node) to a zone.Zone zoneWithContext(Context context)
-> binds a specific version to a fork of the current zoneContextToken attach(Context context)
-> stacks a specific version to the current zoneWith the new APIs, retrieving the active context follows a hierarchy:
Starting a span is the same: a manually specified context is used if given and the active context is used otherwise. The changed part is that the active context might be an attached context.
How Has This Been Tested?
The provided example and unit tests demonstrates functionality. Additional testing will be carried out by consuming the changes in various test applications.
Checklist: